Add CUDA-aware MPI halo exchange for MPMesh#88
Conversation
Shahrear2000
requested review from
Angelyr,
cwsmith,
dhyan1272 and
onkarsahni
as code owners
cwsmith
left a comment
cwsmith
left a comment
There was a problem hiding this comment.
I didn't see anything that was CUDA specific so I'd suggest replacing 'CUDA' in the function/structure/etc. names with 'GPU' for portability to AMD and Intel GPUs.
It looked like one of the files was mostly formatting changes. Including them in the PR makes it hard to find the critical changes.
I also complained about the function names having a suffix with a number and another with 'improved'.
There was also no test case added for this functionality. Any unit test that exercises this code would be a significant improvement.
Note, I didn't read the gpu mpi exchange code in detail. If feedback at that level is needed I can take a look.
| ) | ||
|
|
||
| add_library(polyMPO-core ${SOURCES}) | ||
| target_compile_definitions(polyMPO-core PUBLIC CUDA_AWARE_MPI) |
There was a problem hiding this comment.
Naming this 'GPU_AWARE_MPI' would be more portable (i.e., to AMD and Intel GPUs).
| int numVertices = p_mesh->getNumVertices(); | ||
| auto vtxFieldVel = p_mesh->getMeshField<polyMPO::MeshF_Vel>(); | ||
| mpMesh->communicate_and_take_halo_contributions1(vtxFieldVel, numVertices, 2, 1, 1); | ||
| mpMesh->communicate_and_take_halo_contributions1_improved(vtxFieldVel, numVertices, 2, 1, 1); |
There was a problem hiding this comment.
I suggest removing the '1' and replacing 'improved' with something meaningful.
| void startCommunication(); | ||
|
|
||
| void communicate_and_take_halo_contributions(const Kokkos::View<double**>& meshField, int nEntities, int numEntries, int mode, int op); | ||
| void communicate_and_take_halo_contributions( |
| //communicateFields1(fieldData1, nEntities, numEntries, mode, recvIDVec, recvDataVec); | ||
| communicateFields1(reconVals_host, nEntities, numEntries, mode, recvIDVec, recvDataVec); | ||
|
|
||
| communicateFields1( |
There was a problem hiding this comment.
In general, adding a number to an API (i.e., the 1 at the end) is just going to create problems in the long term for anyone but the person who wrote them.
| const ViewType& fieldData, | ||
| const int numEntities, | ||
| const int numEntries, | ||
| int mode, |
There was a problem hiding this comment.
these formatting only changes should ideally not be part of this PR
| } | ||
| } | ||
|
|
||
| Kokkos::fence(); |
| const int vertex = recvIDGPU(i); | ||
|
|
||
| for(int k = 0; k < numEntries; k++){ | ||
| #ifdef POLYMPO_ASSUME_UNIQUE_HALO_CONTRIBS |
There was a problem hiding this comment.
describing this flag in the docstring for the function would be a good idea
2 participants
Main changes:
Runtime notes: